Accept browser session name wherever a session ID is taken - #178
Conversation
Bump @onkernel/sdk to 0.98.0, which renames the browser sub-resource path parameter to id_or_name, and describe every session_id input on the browser tools and the debug prompt as ID or name. manage_browsers gains name and tags on create and update so a session can be given the name it is later addressed by. Adapt the replay stop call to the renamed SDK parameter.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
bmsaadat
left a comment
bmsaadat
left a comment
There was a problem hiding this comment.
Review: request changes (two wording fixes, code is good)
Reviewed against the API at the deployed #3731 merge commit. The code change is minimal and correct: the SDK bump only renames path parameters and adds the webmcp resource, replays.stop is the sole call the rename touches, and name/tags forwarding matches how neighbouring scalar params are handled. bun test (280 pass), bunx tsc --noEmit, check:managed-auth-app, and a frozen-lockfile install all pass locally. #3731 has been live in prod since 19:05 UTC today, so there is no deploy-ordering concern.
Requested changes
-
Deleted sessions cannot be addressed by name, but the descriptions imply they can.
src/lib/mcp/tools/browsers.ts:436pairs "diagnose active or deleted sessions" with "addressed by ID or by name", and the debug prompt (src/lib/mcp/prompts.ts:101plus the body) says telemetry works after deletion. On the API, the telemetry route resolves throughquerySessionByIDOrName, whose name branch isDeletedAtIsNil(covered byTestGetSessionByIDOrName/"telemetry read resolver accepts a name"), while the ID branch still reads the archive. So the onemanage_browsersaction where deleted sessions matter is exactly where a name returns a generic 404 "browser not found".
Suggestedsession_idtext: "Browser session ID or name. A name resolves only a live session; for a deleted session pass its ID (get_telemetry)." Same caveat on the prompt argument. -
README over-claims name support.
README.md:307says every tool that takes asession_idaccepts the name.manage_browser_poolsreleasetakes asession_idbody field that the API matches by strict ID equality (browser_pools.gorelease handler), so a name is a 404 "session not found in pool". Thenamefield text atbrowsers.ts:451("wherever a session_id is accepted") has the same problem. Suggest scoping both to the browser tools; optionally mark the pool release param as ID only.
Non-blocking
namedescription could state the charset (letters, digits,.,_,-, 1-255 chars, not cuid-like) so an agent's first guess like "Checkout Flow" isn't a 400 round trip.- Add an update test for
name: ""clearing, since that's the advertised behavior;tags: {}is covered. tagsis accepted onlistand silently ignored. The SDK's list params supporttagsandquery; wiring them would let an agent find a session by tag or name it doesn't already know. Fine as a follow-up.- "the name given at creation" could be "or set on update".
Also verified
- Lockfile
nextspecifier resync matchespackage.jsonon main; resolved package unchanged. - kernel CLI v0.33.0 resolves the session argument via GET by id-or-name before every sub-resource call, so the debug prompt's CLI commands already work with a name.
manage_replays listdoes resolve a deleted session's name (replay reads are deleted-inclusive on the API);get,update,delete, replay start/stop are live-only by name.- Prettier warnings on the touched files pre-exist on main; CI doesn't run the format check.
A name resolves only a live session on the API, so get_telemetry on a deleted session needs its ID; say so on the session_id input, the debug prompt, and the tool description. Scope the README claim to the browser tools and mark pool release as ID only. State the name charset, wire the list action's query and tags filters, and cover name clearing on update.
|
Both requested changes and the non-blocking items are in f8b43bf. 1. Deleted sessions by name. 2. README over-claim. Scoped to the six browser tools and states that deleted sessions and Non-blocking
|
bmsaadat
left a comment
bmsaadat
left a comment
There was a problem hiding this comment.
Re-review: approve
Re-checked f8b43bf against the API source rather than the summary. Everything from the last round is in and accurate:
- The live-only caveat for names is on
session_id, themanage_browsersdescription, the debug prompt argument, and the prompt's telemetry section, and it matches how the telemetry route resolves names. - README and the
namefield are scoped to the browser tools;manage_browser_poolsrelease says ID only. - Name charset text matches the API validator, including the cuid rule.
- Update test covers
name: ""clearing; new list test coversqueryandtagsforwarding.
Locally: bun test 281 pass, tsc clean, check:managed-auth-app pass. CI, Bugbot, and Vercel review are green.
One nit before merging: README.md:307 and the name description in src/lib/mcp/tools/browsers.ts:451 list execute_shell_command, but the tool is registered as exec_command (src/lib/mcp/tools/shell.ts:29, and the README's own standalone list uses exec_command). Rename and it's good to go.
Optional, no change needed: the README's "deleted sessions take the ID only" is slightly conservative, since manage_replays list also resolves a deleted session's name on the API. It's safe guidance as written.
2 participants
Summary
The Kernel API now resolves every
/browsers/{id_or_name}/...route by session ID or session name (kernel/kernel#3731, released in@onkernel/sdk0.98.0). This PR brings the MCP server in line so an agent can work with named sessions end to end.@onkernel/sdkbumped^0.97.0->^0.98.0. The sub-resource methods are positional, so the only code change the rename forced isreplays.stop, whose params object field is nowid_or_name.session_idinput is described as "Browser session ID or name":manage_browsers,computer_action,execute_playwright_code,execute_shell_command,browser_curl,manage_replays, plus thedebug-browser-sessionprompt. Values were already forwarded unchanged to the SDK, so names work against the API without further changes.manage_browsersgainsnameandtagsoncreateandupdate, forwarded toBrowserCreateParams/BrowserUpdateParams. On update, an emptynameclears the name and{}clears tags, matching the API. Without this an agent could not create a session by name in the first place.manage_browsersand the README note that sessions can be addressed by ID or name.Testing
New tests in
browsers.test.ts:name/tagspass through create and update, and a name given assession_idreachesretrieve/deleteByIDunchanged.bun test(280 pass),bunx tsc --noEmit,bun run check:managed-auth-app, and the OAuth Redis recordings check pass locally.bun.lock: besides the SDK bump, bun re-synced the workspace entry fornextfrom16.2.11to^16.2.11to matchpackage.json.bun install --frozen-lockfilepasses.